feat: consolidate requirement verdict computation across status and MCP#414
Merged
Conversation
jimisola
commented
Jun 22, 2026
jimisola
left a comment
jimisola
left a comment
Member
Author
There was a problem hiding this comment.
Full PR Review — Consolidated Findings
Scope: docs-only (OpenSpec proposal/design/tasks/spec-delta + CLAUDE.md note + OpenSpec config scaffold). No code changes. Every factual claim in design.md was verified against the repo at the PR HEAD — all referenced symbols exist and MCP_0005 is the correct next ID. openspec validate --strict passes.
| # | Prio | Location | Finding | Fix |
|---|---|---|---|---|
| 1 | Medium | specs/mcp/spec.md:3-7 |
Spec delta is a tautological placeholder ("The system SHALL implement MCP_0005" / "SHALL pass SVC_MCP_0005"). It carries none of the behavioral requirement that proposal.md describes. openspec validate --strict passes (structure-only), so the stub slips through. |
Write a real requirement + WHEN/THEN scenario expressing that the per-requirement MCP status tools report a verdict/structure identical to the status command, from one shared computation. |
| 2 | Low | CLAUDE.md:164 |
"silently drifted and caused bugs (#411, #412)" — #412 is the open consolidation tracking issue (this work), not a bug caused by drift; only #411 was the drift bug. | Reword to "caused bug #411; consolidation tracked in #412". |
| 3 | Low | tasks.md:210 |
Task 5.2 hardcodes "72/72 complete"; tasks 1.1/1.2 add a new requirement, so the count is stale on arrival and brittle. | Use "all requirements complete with SVC_MCP_0005 covered" instead of a fixed count. |
| 4 | Low | design.md/proposal.md |
Refer to the details.py function as get_requirements_status, but the impl is get_requirements_status_all (only the MCP tool is get_requirements_status). tasks.md uses the correct name. |
Align prose with get_requirements_status_all. |
| 5 | Info | openspec/config.yaml |
Committed as the default scaffold with all context/rules commented out — optional opportunity to populate context:. |
Optional; fine to leave as standard scaffold. |
Net: clean, well-reasoned proposal. #1 is the only near-blocking item — the spec delta should express the real requirement since that file survives into the archive.
Automated — /x:full-pr-review
jimisola
commented
Jun 22, 2026
jimisola
left a comment
jimisola
left a comment
Member
Author
There was a problem hiding this comment.
Correction to the review above
After checking the existing openspec/specs/mcp/spec.md, two of my findings were wrong — retracting them:
- #1 (spec delta "placeholder") — RETRACTED. The existing mcp spec uses the exact same
The system SHALL implement MCP_000X/SHALL pass SVC_MCP_000Xform by design: its Purpose note states requirement/SVC content is owned by reqstool (SSOT) and injected at read time viareqstool enrich. TheMCP_0005delta correctly follows that convention; there is nothing to fix. - #4 (function name) — RETRACTED.
design.md/proposal.mdcorrectly name the MCP tools (get_requirements_status);tasks.mdcorrectly names the impl function (get_requirements_status_all). Both accurate in context.
Applied fixes (committed to the branch):
- #2
CLAUDE.md— reworded "caused bugs (#411, #412)" → "caused bug #411 (consolidation tracked in #412)", since #412 is the tracking issue, not a drift-caused bug. - #3
tasks.md5.2 — replaced the hardcoded "72/72 complete" with "all requirements complete with SVC_MCP_0005 covered".
#5 (config.yaml scaffold) left as-is (optional). openspec validate --strict still passes.
Automated — /x:full-pr-review
Adds the OpenSpec proposal/design/tasks for unifying the per-requirement "complete" verdict behind a single computation, replacing the duplicate predicate in common/queries/details.py that has already drifted from StatisticsService once (#411). Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
jimisola
force-pushed
the
refactor/consolidate-requirement-verdict
branch
from
77d1225 to
6eeee17
Compare
…, shared serializer, flag parity, LSP constraint Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
Extracts compute_requirement_status() and the shared _requirement_to_dict() serializer so StatisticsService (status/report/export) and the MCP get_requirement_status/get_requirements_status tools derive the "complete" verdict from one predicate instead of two that had already drifted (#411). Deletes the duplicate _compute_meets/_build_automated_test_summary path in details.py, exposes include_post_build on the MCP tools for parity with `status --with-post-tests`, and adds MCP_0005/SVC_MCP_0005 plus an agreement test covering the previously divergent REQ_ext002_300. Closes #412 Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
jimisola
changed the title
Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
jimisola
commented
Jun 23, 2026
jimisola
left a comment
jimisola
left a comment
Member
Author
There was a problem hiding this comment.
Full PR Review — Consolidated Findings
7 checks run: review, code-review, smells, advanced-smells, security, cruft, testing. Security: no findings.
| # | Prio | Location | Finding | Fix |
|---|---|---|---|---|
| 1 | High | docs/modules/ROOT/pages/mcp.adoc:177,189,194-206 |
Docs still describe the deleted MCP shape (implementation, test_summary, meets_requirements) — actively misleading for MCP API consumers. |
Update to the new shape and document include_post_build. |
| 2 | High | tests/unit/reqstool/common/queries/test_details.py:959-983 |
include_post_build parametrized test can't prove the flag changes anything — no SVC in the fixture sets phase, so both modes select the identical SVC set. |
Add a fixture SVC with phase: post-build and assert behavior actually differs. |
| 3 | High | src/reqstool/storage/requirements_repository.py:131-136 |
New get_svc() — load-bearing for every verdict — has zero test coverage, including the None path. |
Add a unit test for found/not-found. |
| 4 | Medium | statistics_service.py:22, details.py:8 |
_requirement_to_dict is underscore-prefixed yet listed in __all__ and imported across a module boundary. (4/7 checks) |
Rename to requirement_to_dict. |
| 5 | Medium | statistics_service.py:101-130 |
N+1 query pattern: per-requirement, per-SVC get_svc() round-trips replace 4 bulk fetches. Indexed, so sub-ms at today's scale per design doc. (4/7 checks) |
No fix required now; optional follow-up if profiling shows it matters. |
| 6 | Medium | statistics_service.py:170-183 |
_compute_requirement_automated_stats double-queries annotations (checks truthiness, then get_test_results_for_svc re-fetches them internally). |
Reuse the already-fetched annotations list inline. |
| 7 | Medium | statistics_service.py:101 |
compute_requirement_status has zero direct unit tests; the TypeError branch in _check_implementation is untested anywhere. |
Add direct unit tests covering not_applicable, missing MVR/test, and the TypeError branch. |
| 8 | Medium | test_details.py:961-983 |
The new agreement test only proves both callers agree — they call the same predicate now, so it could pass trivially even with a predicate bug. | Add assertions on known expected values for a specific requirement. |
| 9 | Medium | test_details.py:167 |
_make_db_with_req default verification change (MANUAL_TEST→AUTOMATED_TEST) silently shifts which gate several pre-existing tests exercise. |
Document/assert which gate each test exercises; cover the now-orphaned MANUAL_TEST+MVR combination explicitly. |
| 10 | Low | statistics_service.py:101 |
req parameter is untyped while siblings are. |
Annotate as req: RequirementData. |
| 11 | Low | test_details.py |
Test names retain old meets_requirements/compute_meets vocabulary. |
Rename to test_completed_*/test_verdict_*. |
| 12 | Low | mcp/server.py:69,109 |
No negative-path test for include_post_build combined with an invalid id. |
Parametrize the not-found test over include_post_build. |
Not flagged (verified, no action needed): get_svc() SQL correctly parameterized; compute_requirement_status faithfully replicates pre-PR StatisticsService logic; no dead code left from the deleted predicate; include_post_build plumbing proportionate; no security findings.
Fixes for all of the above are being pushed to this branch.
Automated — /x:full-pr-review
…tion - docs/modules/ROOT/pages/mcp.adoc: update stale MCP status tool docs to the new shape (completed/implementation_type/automated_tests/manual_tests) and document include_post_build, replacing the deleted meets_requirements/ test_summary/implementation shape. - Rename _requirement_to_dict -> requirement_to_dict: it's a deliberate shared serializer imported across module boundaries, not module-private. - requirements_repository.py: extract get_test_results_for_annotations so _compute_requirement_automated_stats reuses the annotation list it already fetched instead of re-querying it inside get_test_results_for_svc. - Type the req parameter on compute_requirement_status. - Add direct unit tests for compute_requirement_status (not_applicable, missing MVR/test, completed, and the TypeError guard), for the new RequirementsRepository.get_svc() getter (found/not-found), and a dedicated test proving include_post_build actually changes the verdict (the existing fixture never varies SVC phase, so the prior agreement test alone couldn't prove this). - Strengthen the cross-path agreement test with known-correct expected values for REQ_ext002_300, not just cross-path equality. - Rename test_meets_requirements_*/test_compute_meets_* to test_completed_* to match the new vocabulary, and document which gate (automated_tests vs manual_tests) each _make_db_with_req-based test exercises. - Add a negative-path test for get_requirement_status with an invalid id combined with include_post_build. Addresses findings from the automated full-PR-review on #414. Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
jimisola
commented
Jun 23, 2026
jimisola
left a comment
jimisola
left a comment
Member
Author
There was a problem hiding this comment.
Full PR Review — Round 2 (commit e1a0145)
All 12 round-1 findings verified as correctly and completely fixed. 854 unit + 17 integration tests pass; flake8/black clean. Round 2 surfaced 2 new, minor follow-ups the fix commit's scope didn't reach:
| # | Prio | Location | Finding | Fix |
|---|---|---|---|---|
| 1 | Medium | src/reqstool/common/queries/details.py:91-92 (get_urn_details) |
Same duplicate-query pattern eliminated in compute_requirement_status still exists here: fetches test_annotations then calls get_test_results_for_svc(svc.id), which re-fetches the same annotations internally. |
Change to repo.get_test_results_for_annotations(svc.id.urn, test_annotations). |
| 2 | Low | src/reqstool/storage/requirements_repository.py:270 (get_test_results_for_annotations) |
New repository method has no direct unit test — only exercised transitively via compute_requirement_status tests. Its sibling get_svc got a direct found/not-found pair in the same commit; this one didn't. |
Add a direct unit test covering the CLASS and METHOD annotation branches. |
Everything else checked out clean: all 6 round-1 test-coverage fixes verified substantive (including mutation-testing the include_post_build test to confirm it's load-bearing), the _requirement_to_dict → requirement_to_dict rename is complete repo-wide, docs are consistent, no new cruft, no security findings, no over-engineering.
Fixes for both are being pushed to this branch.
Automated — /x:full-pr-review
- get_urn_details (details.py) still had the duplicate-annotations-query pattern that get_test_results_for_annotations was extracted to fix; route it through the shared method like compute_requirement_status does. - Add direct unit tests for get_test_results_for_annotations (METHOD found, METHOD missing, CLASS aggregation) and a test pinning that get_test_results_for_svc delegates to it without behavior drift. Signed-off-by: Jimisola Laursen <jimisola@jimisola.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
consolidate-requirement-verdict(proposal, design, spec delta, tasks) and implements it.compute_requirement_status()(per-requirement implementation/automated/manual verdict) and a shared_requirement_to_dict()serializer instatistics_service.py, queried through the repository's scoped per-requirement getters.StatisticsService(status/report/export/MCPget_status) and the MCPget_requirement_status/get_requirements_statustools now derive their verdict from this single predicate, instead of two independent implementations that had already drifted once (fix(mcp): correctly flag skipped/missing automated tests as unmet #411)._compute_meets/_build_automated_test_summarypredicate fromcommon/queries/details.py.include_post_build(defaultFalse) on the MCP status tools for parity withstatus --with-post-tests.get_requirement_status/get_requirements_statusnow emit the unified status shape (completed,implementation_type,automated_tests/manual_tests), replacing the oldmeets_requirements/flattest_summaryshape. Some verdict values also change for requirements with post-build-only SVCs, not-applicable cases, and unrecorded test executions — these now matchstatusexactly.MCP_0005/SVC_MCP_0005to the reqstool SSOT and a test assertingget_requirement_status/get_requirements_statusagree withStatisticsServicein both scoping modes, covering the previously divergentREQ_ext002_300.Closes #412
Test plan
hatch run dev:pytest --cov=reqstool— 940 passedhatch run dev:flake8— cleanstatus/reportontest_standard/test_basicfixtures) — byte-identical tomainreqstool status local -p docs/reqstool— 72/72 complete,SVC_MCP_0005coveredopenspec validate --all --strict— all pass